-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apply transform to "region" bounding volume type #6755
Conversation
@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
4ef4991
to
f717788
Compare
Updated. @ggetz can you review? Modified an existing test to check that a tileset with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing with RTC_CENTER tilesets now, are we applying the transforms in the correct order there?
Source/Scene/Cesium3DTile.js
Outdated
function createRegion(region, transform, initialTransform, result) { | ||
if (!Matrix4.equalsEpsilon(transform, initialTransform, CesiumMath.EPSILON8)) { | ||
return createBoxFromTransformedRegion(region, transform, initialTransform, result); | ||
} | ||
|
||
if (defined(result)) { | ||
// Don't update regions when the transform changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed?
var rectangleRegion = Rectangle.unpack(region, 0, scratchRectangle); | ||
function createBoxFromTransformedRegion(region, transform, initialTransform, result) { | ||
var rectangle = Rectangle.unpack(region, 0, scratchRectangle); | ||
var minimumHeight = region[4]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we just created an instance of a Rectangle, I think it would be more readable to use east
, north
, etc class members rather than the accessor. Here and in createRegion
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for the region[4]
and region[5]
is Rectangle
doesn't contain min/max height properties.
Ah sorry for not catching that earlier. It was a small change in the test. The tile was out of view and never updated its |
Thanks for the review @ggetz! Should be ready now. |
Thanks @lilleyse! |
I modified the '3D Tiles Adjust Height.html' in the Apps/Sandcastle/gallery, i want to rotate the 3D Tiles,the code works in 1.47 but in 1.48 and 1.49 the 3D Tiles disappear when i rotate the 3D Tiles. Is this related to the change? the code is
|
Hey @liujingjie, it wasn't this change that caused the problem. But the tileset in |
Looking at this closer the difference in matrix order is intentional.
In general it is much easier to transform a tileset if it uses a tile transform on the root tile rather than |
I also encountered the same problem. Versions prior to 1.47 can be used, and later versions, 3dtiles model rotation and resizing will not work. I use Google translate, because I cannot understand English. The explanation is too complicated (Sorry). Is there any chance you can provide the correct code, or point out which part of the code is wrong? Thanks a lot! |
Ok.. it looks like this PR did break the previous behavior, though I think the previous behavior was working accidentally. The quick workaround is in
to
This fixes the example tileset in 1.47 and any other tilesets that use |
thanks so much! i will give it a try |
哥们,搞定了吗,这个问题,向您请教下 |
哥们,问题解决了吗 |
Fixes #5984
Local Sandcastle link
Before / after
As noted in the Sandcastle comments, this will break tilesets that expect the geometry to be transformed but not the region. We have one test tileset that does this, but I don't think I've ever seen this is practice.This will also require changes to the 3D Tiles spec - this section disallows the transform from being applied to regions.To do:
Spec update - Remove tile transform restriction on regions 3d-tiles#326